Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor routing primitive #9543

Merged
merged 12 commits into from
Jan 22, 2022
Merged

Conversation

harshit-gangal
Copy link
Member

@harshit-gangal harshit-gangal commented Jan 20, 2022

Description

This PR refactors the route, update and delete engine primitive to use common routing logic. This makes it simpler to change the routing logic in one place now.

Related Issue(s)

Checklist

  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

@harshit-gangal harshit-gangal marked this pull request as ready for review January 21, 2022 14:35
@harshit-gangal harshit-gangal changed the title Refactor routing Refactor routing primitive Jan 21, 2022
)

// Opcode is a number representing the opcode
// for any engine primitve.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: typo primitve, and it doesn't represent the opcode for any engine primitive, only the routing ones

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will fix the comment in follow up PR

Copy link
Member

@GuptaManan100 GuptaManan100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from Andres's comment, I think everything else looks great!

@harshit-gangal harshit-gangal merged commit 3013b52 into vitessio:main Jan 22, 2022
@harshit-gangal harshit-gangal deleted the refactor-routing branch January 22, 2022 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants